ddl:support alter table drop partition #6460

ciscoxll · 2018-05-04T03:26:11Z

alter table drop partition

The first step to implement alter table drop partition.

XuHuaiyu · 2018-05-04T05:58:48Z

parser/parser.y

@@ -936,6 +936,13 @@ AlterTableSpec:
 	{
 		$$ = &ast.AlterTableSpec{Tp: ast.AlterTableDropPrimaryKey}
 	}
+|   "DROP" "PARTITION" Identifier


add test for this.

alivxxx · 2018-05-07T03:43:49Z

ddl/ddl_api.go

+	}
+	t, err := is.TableByName(ident.Schema, ident.Name)
+	if err != nil {
+		return errors.Trace(infoschema.ErrTableExists.GenByArgs(ident.Schema, ident.Name))


Why ErrTableExists?

tiancaiamao · 2018-05-08T07:30:26Z

ddl/table.go

@@ -394,3 +394,67 @@ func updateVersionAndTableInfo(t *meta.Meta, job *model.Job, tblInfo *model.Tabl
 	}
 	return ver, t.UpdateTable(job.SchemaID, tblInfo)
 }
+
+func checkPartitionRangeList(meta *model.TableInfo, partName string) error {
+	if meta.Partition != nil {


if meta.Partition == nil { return errors.Trace(infoschema.ErrPartitionMgmtOnNonpartitioned) } ...

tiancaiamao · 2018-05-08T07:32:23Z

ddl/table.go

+
+func checkPartitionRangeList(meta *model.TableInfo, partName string) error {
+	if meta.Partition != nil {
+		noExist := false


I think you mean exist := false ?

tiancaiamao · 2018-05-08T07:35:40Z

ddl/table.go

+		parInfo.Definitions = make([]model.PartitionDefinition, 0, 1)
+	}
+	for _, oldDef := range oldDefs {
+		if oldDef.Name == partName {


Is partition name case sensitive of not?

tiancaiamao · 2018-05-08T07:44:29Z

ddl/table.go

+	return nil
+}
+
+func dropTablePartitionInfo(meta *model.TableInfo, partName string) error {


What will happen if we drop the last partition? Will the table degenerate to a normal table?
What happen when we add a partition back? Will the partition id be the same?

tiancaiamao · 2018-05-08T07:45:37Z

ddl/table.go

+	if err != nil {
+		return errors.Trace(err)
+	}
+	parInfo := &model.PartitionInfo{}


I guess you don't need to define a parInfo, just Definitions is enough?

ciscoxll · 2018-06-14T11:09:42Z

@zimulala @tiancaiamao PTAL

tiancaiamao · 2018-06-19T07:33:34Z

ddl/table.go

@@ -26,6 +26,7 @@ import (
 	"github.com/pingcap/tidb/table"
 	"github.com/pingcap/tidb/tablecodec"
 	log "github.com/sirupsen/logrus"
+	"strings"


Move to line 18

tiancaiamao · 2018-06-19T07:39:26Z

ddl/table.go

+		} else {
+			newDefs = append(newDefs, oldDef)
+		}
+	}


Some useful trick

Cut a = append(a[:i], a[j:]...)

https://github.com/golang/go/wiki/SliceTricks

tiancaiamao · 2018-06-19T09:25:48Z

ddl/table.go

+	partInfo := tblInfo.Partition
+	originalState := partInfo.State
+	switch partInfo.State {
+	case model.StatePublic:


I think you have to re-design here.
The problem drop index faces, is that not all TiDB nodes see the same state.
If tidb1 think a table has an index, while tidb2 think a table hasn't an index, they will run into trouble.
That why we need several steps and states for one alter table operation.

Table partition is different.

partition by range (id) (
p1 less than (10),
p2 less than (30),
p3 less than (maxvaluex)
)

alter table drop partition p2

Insert into t value (20)
It should locate to p3, how to keep the change atomic ?

ciscoxll · 2018-06-19T11:48:40Z

@zimulala @tiancaiamao PTAL

tiancaiamao · 2018-06-20T03:13:38Z

meta/meta.go

+		return errors.Trace(err)
+	}
+	if delAutoID {
+		if err := m.txn.HDel(dbKey, m.autoTableIDKey(pid)); err != nil {


autoTableIDKey belongs to table or partition? @zimulala

ciscoxll · 2018-06-27T11:00:59Z

@zimulala @tiancaiamao @winkyao PTAL

ciscoxll · 2018-06-28T03:44:19Z

@zimulala @tiancaiamao @winkyao PTAL

tiancaiamao · 2018-06-29T02:19:51Z

ddl/ddl.go

@@ -168,6 +168,8 @@ var (
 	ErrRangeNotIncreasing = terror.ClassSchema.New(codeRangeNotIncreasing, "VALUES LESS THAN value must be strictly increasing for each partition")
 	// ErrPartitionMaxvalue returns maxvalue can only be used in last partition definition.
 	ErrPartitionMaxvalue = terror.ClassSchema.New(codePartitionMaxvalue, "MAXVALUE can only be used in last partition definition")
+	//ErrDropLastPartition returns cannot remove all partitions, use drop table instead.
+	ErrDropLastPartition = terror.ClassSchema.New(codeDropLastPartition, "Duplicate partition name %s")


Why ErrDropLastPartition message is "Duplicate partition name" ??

tiancaiamao · 2018-06-29T02:32:00Z

ddl/ddl_api.go

+		return errors.Trace(infoschema.ErrTableNotExists.GenByArgs(ident.Schema, ident.Name))
+	}
+	meta := t.Meta()
+	if meta.GetPartitionInfo() == nil && meta.Partition == nil {


meta.GetPartitionInfo() == nil is enough

tiancaiamao · 2018-06-29T02:41:11Z

ddl/ddl_db_test.go

@@ -2366,7 +2366,7 @@ func (s *testDBSuite) TestAlterTableAddPartition(c *C) {

 	ctx := s.tk.Se.(sessionctx.Context)
 	is := domain.GetDomain(ctx).InfoSchema()
-	tbl, err := is.TableByName(model.NewCIStr("test"), model.NewCIStr("tp"))


The old code success? why?

Because the table tp exists and is a partitioned table.

tiancaiamao · 2018-06-29T02:44:59Z

ddl/ddl_db_test.go

+
+func (s *testDBSuite) TestAlterTableDropPartition(c *C) {
+	s.tk.MustExec("use test")
+	s.tk.MustExec("drop table if employees")


s.tk.MustExec("set @@tidb_enable_table_partition = 1")

I guess you test doesn't cover your code, because Partition.Enable is false by default.

tiancaiamao · 2018-06-29T02:47:56Z

ddl/ddl_db_test.go

+	tbl, err := is.TableByName(model.NewCIStr("test"), model.NewCIStr("employees"))
+	c.Assert(err, IsNil)
+	c.Assert(tbl.Meta().Partition, NotNil)
+	part := tbl.Meta().Partition


tbl.Meta().GetPartitionInfo()

Follow this way, and when we set Partition.Enable to true by default, the code should works.

tiancaiamao · 2018-06-29T02:54:06Z

ddl/ddl_db_test.go

+	is = domain.GetDomain(ctx).InfoSchema()
+	tbl, err = is.TableByName(model.NewCIStr("test"), model.NewCIStr("t4"))
+	c.Assert(err, IsNil)
+	c.Assert(tbl.Meta().Partition, NotNil)


tiancaiamao · 2018-06-29T02:55:57Z

ddl/delete_range.go

@@ -241,6 +241,11 @@ func insertJobIntoDeleteRangeTable(ctx sessionctx.Context, job *model.Job) error
 		startKey := tablecodec.EncodeTablePrefix(tableID)
 		endKey := tablecodec.EncodeTablePrefix(tableID + 1)
 		return doInsert(s, job.ID, tableID, startKey, endKey, now)
+	case model.ActionDropTablePartition:
+		tableID := job.TableID


Could this handle both tikv and mocktikv?

It should be ok.

@tiancaiamao I have tested that it is normal to delete partition data.

Why not use paritionID?

tiancaiamao · 2018-06-29T02:57:13Z

ddl/table.go

@@ -484,3 +484,76 @@ func checkAddPartitionValue(meta *model.TableInfo, part *model.PartitionInfo) er
 	}
 	return nil
 }
+
+func checkPartitionRangeList(meta *model.TableInfo, partName string) error {
+	if meta.Partition == nil {


GetPartitionInfo

ciscoxll · 2018-06-29T07:19:40Z

@tiancaiamao @zimulala @coocood PTAL.

ciscoxll · 2018-07-02T06:36:27Z

@tiancaiamao @zimulala @coocood PTAL.

shenli · 2018-07-03T03:48:48Z

ddl/ddl_api.go

@@ -1007,6 +1007,8 @@ func (d *ddl) AlterTable(ctx sessionctx.Context, ident ast.Ident, specs []*ast.A
 			err = d.AddColumn(ctx, ident, spec)
 		case ast.AlterTableAddPartitions:
 			err = d.AddTablePartitions(ctx, ident, spec)
+		case ast.AlterTableDropPartition:


Keep the cases in alphabet order.

shenli · 2018-07-03T03:49:24Z

ddl/ddl_worker.go

@@ -435,6 +435,8 @@ func (w *worker) runDDLJob(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64,
 		ver, err = onModifyTableComment(t, job)
 	case model.ActionAddTablePartition:
 		ver, err = onAddTablePartition(t, job)
+	case model.ActionDropTablePartition:


shenli · 2018-07-03T03:51:29Z

ddl/table.go

+	oldDefs := meta.Partition.Definitions
+	for _, def := range oldDefs {
+		if strings.EqualFold(def.Name, partName) {
+			if len(oldDefs) == 1 {


Do we need to check this in DDL worker?

@shenli Parallel drop partition needs to be checked.

shenli · 2018-07-03T03:54:18Z

meta/meta.go

+		return errors.Trace(err)
+	}
+
+	tableKey := m.tableKey(pid)


Is there a separated partition info outside table info?

ciscoxll · 2018-07-04T02:48:41Z

@tiancaiamao @zimulala @coocood PTAL.

tiancaiamao · 2018-07-04T07:28:32Z

ddl/ddl_db_test.go

+	s.testErrorCode(c, sql2, mysql.ErrDropPartitionNonExistent)
+
+	s.tk.MustExec("drop table if t3")
+	s.tk.MustExec(`"CREATE TABLE t3 (


`" Is this a normal SQL string? "`

shenli · 2018-07-04T07:38:57Z

ddl/table.go

+	newDefs = make([]model.PartitionDefinition, 0, len(oldDefs)-1)
+	var pid int64
+	for i := 0; i < len(oldDefs); i++ {
+		if strings.EqualFold(oldDefs[i].Name, partName) {


if !strings.EqualFold(oldDefs[i].Name, partName) {
continue;
}
BTW: I think we should use CIStr for ldDefs[i].Name.

shenli · 2018-07-04T07:42:35Z

ddl/ddl_api.go

+		Args:       []interface{}{spec.Name},
+	}
+
+	err = d.doDDLJob(ctx, job)


Check this error.

shenli · 2018-07-04T07:43:26Z

ddl/delete_range.go

@@ -241,6 +241,11 @@ func insertJobIntoDeleteRangeTable(ctx sessionctx.Context, job *model.Job) error
 		startKey := tablecodec.EncodeTablePrefix(tableID)
 		endKey := tablecodec.EncodeTablePrefix(tableID + 1)
 		return doInsert(s, job.ID, tableID, startKey, endKey, now)
+	case model.ActionDropTablePartition:
+		tableID := job.TableID


Why not use paritionID?

shenli · 2018-07-04T07:44:46Z

ddl/table.go

+	// Finish this job.
+	job.FinishTableJob(model.JobStateDone, model.StateNone, ver, tblInfo)
+
+	return ver, errors.Trace(err)


The err must be nil here.

shenli · 2018-07-04T07:46:10Z

ddl/table.go

@@ -484,3 +484,73 @@ func checkAddPartitionValue(meta *model.TableInfo, part *model.PartitionInfo) er
 	}
 	return nil
 }
+
+func checkPartitionRangeList(meta *model.TableInfo, partName string) error {


Add comment for this function.

tiancaiamao · 2018-07-10T09:01:30Z

ddl/delete_range.go

@@ -241,6 +241,12 @@ func insertJobIntoDeleteRangeTable(ctx sessionctx.Context, job *model.Job) error
 		startKey := tablecodec.EncodeTablePrefix(tableID)
 		endKey := tablecodec.EncodeTablePrefix(tableID + 1)
 		return doInsert(s, job.ID, tableID, startKey, endKey, now)
+	case model.ActionDropTablePartition:
+		// the `job.TableID` here stores partitionID.


tiancaiamao · 2018-07-10T10:47:16Z

LGTM

zimulala · 2018-07-10T11:34:26Z

ddl/partition.go

+	tblInfo.Partition.Definitions = newDefs
+}
+
+// onDropTablePartition delete old partition meta.


s/delete/deletes

zimulala · 2018-07-10T11:40:37Z

ddl/db_test.go

+	part := tbl.Meta().Partition
+	c.Assert(part.Type, Equals, model.PartitionTypeRange)
+	c.Assert(part.Expr, Equals, "`hired`")
+


Please remove this blank line.

zimulala · 2018-07-10T11:41:36Z

ddl/db_test.go

+	part = tbl.Meta().Partition
+	c.Assert(part.Type, Equals, model.PartitionTypeRange)
+	c.Assert(part.Expr, Equals, "`id`")
+


zimulala · 2018-07-10T11:43:44Z

ddl/db_test.go

+	s.tk.MustExec("drop table if exists tr;")
+	s.tk.MustExec(` create table tr(
+		id int, name varchar(50), 
+		purchased date


Could we add a key for purchased? And check the index rows.

@zimulala Need to wait for this PR #6814 to complete before adding it.

zimulala · 2018-07-10T12:01:39Z

ddl/partition.go

+	}
+	tblInfo, err := getTableInfo(t, job, job.SchemaID)
+	if err != nil {
+		job.State = model.JobStateCancelled


It has been set in getTableInfo.

zimulala · 2018-07-11T06:55:12Z

ddl/partition.go

+	// Finish this job.
+	job.FinishTableJob(model.JobStateDone, model.StateNone, ver, tblInfo)
+	// A background job will be created to delete old partition data.
+	job.TableID = pid


We'd better not change TableID meaning. It is used by binlog.

zimulala · 2018-07-11T07:03:47Z

ddl/partition.go

+func checkDropTablePartition(meta *model.TableInfo, partName string) error {
+	oldDefs := meta.Partition.Definitions
+	for _, def := range oldDefs {
+		if strings.EqualFold(def.Name.O, partName) {


Do we need to use def.Name.L and partName.L check?

zimulala · 2018-07-11T08:54:22Z

ddl/partition.go

+	newDefs := make([]model.PartitionDefinition, 0, len(oldDefs)-1)
+	var pid int64
+	for i := 0; i < len(oldDefs); i++ {
+		if !strings.EqualFold(oldDefs[i].Name.O, partName) {


Do we check it with oldDefs[i].Name.L?
Could we add a test for this？

zimulala

LGTM

zimulala · 2018-07-11T09:43:04Z

/run-all-tests

ciscoxll added DDL require-LGT3 Indicates that the PR requires three LGTM. labels May 4, 2018

ciscoxll changed the title ~~ddl:support table partition in alter table drop partition stmt~~ ddl:support alter table drop partition stmt May 4, 2018

XuHuaiyu reviewed May 4, 2018

View reviewed changes

alivxxx reviewed May 7, 2018

View reviewed changes

tiancaiamao reviewed May 8, 2018

View reviewed changes

zimulala removed the require-LGT3 Indicates that the PR requires three LGTM. label May 9, 2018

ciscoxll removed the WIP label Jun 11, 2018

ciscoxll force-pushed the drop-table-partition branch 2 times, most recently from 622da70 to 2935cda Compare June 14, 2018 09:59

tiancaiamao reviewed Jun 19, 2018

View reviewed changes

ciscoxll changed the title ~~ddl:support alter table drop partition stmt~~ ddl:support alter table drop partition Jun 19, 2018

tiancaiamao reviewed Jun 20, 2018

View reviewed changes

winkyao added the status/WIP label Jun 21, 2018

ciscoxll force-pushed the drop-table-partition branch 2 times, most recently from b374ed9 to d6c2ced Compare June 27, 2018 10:19

ciscoxll removed the status/WIP label Jun 27, 2018

tiancaiamao reviewed Jun 29, 2018

View reviewed changes

shenli reviewed Jul 3, 2018

View reviewed changes

tiancaiamao reviewed Jul 4, 2018

View reviewed changes

shenli reviewed Jul 4, 2018

View reviewed changes

Address comments

b6f117d

tiancaiamao reviewed Jul 10, 2018

View reviewed changes

Address comments

e493083

ciscoxll force-pushed the drop-table-partition branch from c11e3a4 to e493083 Compare July 10, 2018 10:00

change function comments

0708ecd

ciscoxll added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 10, 2018

zimulala reviewed Jul 10, 2018

View reviewed changes

ciscoxll added 4 commits July 10, 2018 20:30

Address comments

6e524b0

fix the updateTable problem

0c86c04

add removePartitionInfo comments

63fc315

add function comments

3e856ab

ciscoxll force-pushed the drop-table-partition branch from 73f8684 to 3e856ab Compare July 11, 2018 04:16

zimulala reviewed Jul 11, 2018

View reviewed changes

ciscoxll added 2 commits July 11, 2018 16:18

Address comments

ef9063c

Address comments

cf97e71

zimulala reviewed Jul 11, 2018

View reviewed changes

ciscoxll added 2 commits July 11, 2018 17:11

add ErrDropPartitionNonExistent test

8ce934c

Merge branch 'master' into drop-table-partition

b0f421c

zimulala approved these changes Jul 11, 2018

View reviewed changes

zimulala added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Jul 11, 2018

ciscoxll merged commit cc72254 into pingcap:master Jul 11, 2018

ciscoxll deleted the drop-table-partition branch July 11, 2018 10:26

sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

ddl:support alter table drop partition #6460

ddl:support alter table drop partition #6460

Conversation

ciscoxll commented May 4, 2018 • edited Loading

alter table drop partition

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciscoxll commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciscoxll commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciscoxll commented Jun 27, 2018

ciscoxll commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciscoxll commented Jun 29, 2018

ciscoxll commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciscoxll commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

zimulala commented Jul 11, 2018

ciscoxll commented May 4, 2018 •

edited

Loading

zimulala Jul 11, 2018 •

edited

Loading